Skip to content

perf: skip re-fetching a PR's commits when its head hasn't moved - #5

Merged
ttomalak-bkpr merged 1 commit into
masterfrom
feat/incremental-pull-request-commits
Aug 25, 2026
Merged

perf: skip re-fetching a PR's commits when its head hasn't moved#5
ttomalak-bkpr merged 1 commit into
masterfrom
feat/incremental-pull-request-commits

Conversation

@ttomalak-bkpr

Copy link
Copy Markdown
Contributor

Summary

  • pull_request_commits has no replication_key, so it's full_table. Every time PullRequestsStream re-yields a PR - which happens on any change to it (a label, a comment, a review, not just new commits) - this stream fully re-fetches the PR's entire commit list from page 1, every single time, regardless of history already synced.
  • For actively-discussed PRs, this burns rate-limit budget on data that hasn't actually changed at all, and this cost never diminishes over time (it's tied to ongoing PR activity, not backfill progress) - confirmed as the actual cause of tap-github-beekpr exhausting all its GitHub tokens partway through nearly every run (round-robin summary showed processed 36/587 repos before hitting RuntimeError: All GitHub tokens have hit their rate limit).
  • A PR's commit list is immutable for a given head sha - it only changes on a new push/rebase/force-push. This tracks the last-synced head sha per PR (passed down via get_child_context) and skips the fetch entirely when it's unchanged. State is partitioned per repo (existing state_partitioning_keys), so the tracking dict correctly covers every PR in the repo without cross-PR bleed.
  • reviews has the same full_table issue but no equivalent cheap signal (a review can be submitted without any commit change), so it isn't addressed here - flagging as a known follow-up.

Test plan

  • Manual test simulating 4 scenarios against the real class (mocking only the underlying HTTP fetch): new PR fetches, unchanged head_sha skips entirely, a new push (changed head_sha) re-fetches, and a different PR with a coincidentally-identical head_sha is not incorrectly skipped (no cross-PR state bleed).
  • ruff check / ruff format --check - clean (no new issues; pre-existing unrelated line-length issues elsewhere in the file untouched).
  • mypy - clean.
  • Full existing test suite - same 72 passed / 10 pre-existing network-dependent failures as master, no regressions.

pull_request_commits has no replication_key, so it's full_table - every
time PullRequestsStream re-yields a PR (which happens on *any* change to
it: a label, a comment, a review, not just new commits), this stream
fully re-fetches the PR's entire commit list from page 1, forever,
regardless of history already synced. For actively-discussed PRs this
burns through rate limit budget on data that hasn't changed at all.

A PR's commit list is immutable for a given head sha - it only changes
on a new push/rebase/force-push. Track the last-synced head sha per PR
(passed down via child context) and skip the fetch entirely when it's
unchanged. State is partitioned per repo, so the tracking dict covers
every PR in the repo without cross-PR bleed.

reviews has the same full_table issue but no equivalent cheap signal
(a review can be added without any commit change), so it isn't
addressed here.
@ttomalak-bkpr
ttomalak-bkpr requested review from a team and lumautomation as code owners August 19, 2026 16:45
@ttomalak-bkpr
ttomalak-bkpr merged commit 54c2d0a into master Aug 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants